fix(crypto): validate that digest length must be an integer between 0 and isize::MAX #3799
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
These canary test failures don't seem to be related to my changes..? |
iuioiua
suggested changes
Nov 14, 2023
iuioiua
approved these changes
Nov 14, 2023
There was a problem hiding this comment.
LGTM! Thank you, @jeremyBanks. Thorough work, as per usual.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, if we call
crypto.subtle.digest/.digestSyncwith a variable-length hash algorithm and length that is...usize::MAXfor WASM: it will be truncated, usually to0, producing an incorrect result.isize::MAXfor WASM: it will trigger an internalcapacity_overflowerror from Rust'sVec.capacity_overflowerror from Rust'sVec.Some of this behaviour is a result of the JS-WASM translation layer, so this PR adds a check on the JavaScript side to throw a
RangeErrorin these cases, before calling the Rust/WASM (which already checks that the length is valid for the specific algorithm being used).This also resolves #3798, which describes how our tests cases were already expecting errors for these cases, but the test logic had a bug that was causing the assertion errors to be suppressed. This fixes that test logic.
One might argue that this is a
BREAKINGchange since it produces an observable change in behaviour (at least for the non-integer case). I'm not sure it's a significant enough to count, but feel free to rename this PR to indicate that it'sBREAKINGif you disagree.